BUG: evaluate parachute triggers once per time node (#1086) - #1121
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1121 +/- ##
===========================================
+ Coverage 83.53% 83.61% +0.08%
===========================================
Files 130 130
Lines 17086 17072 -14
===========================================
+ Hits 14273 14275 +2
+ Misses 2813 2797 -16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I ran the same never-deploying 10 Hz trigger on current
This confirms that the duplicate path was evaluating the trigger and appending pressure data twice at each node. The head keeps one evaluation and one pressure row per node. The committed regression file also passes independently: Environment: Python 3.12.6; NumPy 2.5.2; pytest 9.1.1; macOS 26.5.2 arm64. |
Remove the duplicate inline parachute loop in Flight.__simulate; keep only __check_and_handle_parachute_triggers.
d062c37 to
6e5aee7
Compare
Conflicts, both from RocketPy-Team#1121 ("evaluate parachute triggers once per time node"), which landed after this branch was cut: - rocketpy/simulation/flight.py: RocketPy-Team#1121 moved the parachute loop out of __simulate into __check_and_handle_parachute_triggers, and in doing so already changed the trigger evaluation time from self.t to node.t. This branch had made the same self.t -> node.t change by hand, so that part of it is now redundant upstream. Resolved to develop's extracted helper; the only thing this branch still changes in flight.py is the one line setting parachute._eval_time. - tests/unit/test_parachute_triggers.py: both sides appended a test to the end of the file. Kept both. Two follow-ups while here: - Parachute.__init__ now declares self._eval_time = None next to _trigger_falling_only and _trigger_needs_height, so the attribute is part of the object's state rather than something Flight conjures onto it. The trigger closure reads self._eval_time directly; the getattr default was unreachable anyway, since triggerfunc only exists once __init__ has run. - docs/user/parachute_triggers.rst gained a "Fixed-time trigger" section. That page documents the "apogee" and numeric-altitude forms and tells readers a burnout delay needs a custom trigger, so leaving the new built-in form out of it was the one real gap. Includes the ("time", motor.burn_out_time + delay) composition and a note that firing at speed produces large loads. Verified: tests/unit 2131 passed, 17 skipped; the new integration test and the three parachute unit modules pass; ruff check and format clean; pylint 10.00/10. Checked in a real flight that ("time", 3.0) and ("time", 8.0) deploy at exactly those times while still ascending (vz +268 and +192 m/s), and that a plain "apogee" chute still fires at apogee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pull request type
Checklist
black rocketpy/ tests//make lint) has passed locallypytest tests -m slow --runslow) have passed locallyCHANGELOG.md— no action needed; an LLM workflow auto-updates it after mergeCurrent behavior
Each time node evaluated parachute triggers twice: an inline loop in
Flight.__simulateand again in__check_and_handle_parachute_triggers.Fixes #1086
New behavior
The inline loop is removed; only
__check_and_handle_parachute_triggersruns per node. Regression test asserts one call per sampled height.Breaking change
Additional information
Focused unit tests added/extended; full slow suite not run in this contribution pass.